refactor: create local stores and UTS - WPB-12100 #2141
Conversation
…ects in storage layer
echoes-hq
bot
added
echoes: technical-roadmap
| func storeConnection( | ||
| _ connectionPayload: Connection | ||
| _ connectionInfo: ConnectionInfo |
There was a problem hiding this comment.
You'll see changes like this in this PR: storage layer should not be aware of API objects so we create a domain model between repository and local store, here's the steps:
- Repo will fetch API model from remote.
- Repo will prepare data for the local store by mapping it to a domain model
- Repo will pass this model to the local store
- Local store will rely that model to manipulate (fetch, update, create) NSManagedObject
There was a problem hiding this comment.
So now, it looks like all NSManagedObjects or context are isolated to Local stores, right?
I wonder if they would be cases where we want to be more flexible : thinking about the save contexts strategy we discussed.
Or going even further isolate all this to a WireStorage package
There was a problem hiding this comment.
yes storage operations are isolated to the local stores.
Moving to a WireStorage package could make sense as it would be consistent with the approach we used in WireAPI that contains UsersAPI ConversationsAPI...
WireStorage could follow that approach with UsersLocalStore ConversationsLocalStore etc and later introduce a generic storage service that our stores would rely on to perform any kind of local operations, just like we did with APIService in WireAPI.
There was a problem hiding this comment.
I think we should consider this still an open topic as it has a lot of implications to the overall architecture that should be further investigated. Having said that, I think that for now it is fine to introduce such isolation since it's generally easier to remove abstractions that it is to put them in place.
There was a problem hiding this comment.
@johnxnguyen you mean the WireStorage move ?
WireDomain/Sources/WireDomain/Repositories/Connections/ConnectionsRepository.swift
Show resolved
Hide resolved
...omain/Sources/WireDomain/Repositories/ConversationsLabels/ConversationLabelsLocalStore.swift
Show resolved
Hide resolved
WireDomain/Sources/WireDomain/Repositories/FeatureConfig/FeatureConfigLocalStore.swift
Show resolved
Hide resolved
WireDomain/Sources/WireDomain/Repositories/FeatureConfig/Models/FeatureState.swift
Show resolved
Hide resolved
WireDomain/Sources/WireDomain/Repositories/Team/Models/TeamMemberInfo.swift
Show resolved
Hide resolved
WireDomain/Sources/WireDomain/Repositories/UpdateEvents/UpdateEventsLocalStore.swift
Show resolved
Hide resolved
WireDomain/Sources/WireDomain/Repositories/User/Models/NewUserInfo.swift
Show resolved
Hide resolved
WireDomain/Sources/WireDomain/Repositories/User/Models/UserUpdateInfo.swift
Show resolved
Hide resolved
WireDomain/Sources/WireDomain/Repositories/UserClients/UserClientsLocalStore.swift
Show resolved
Hide resolved
Test Results165 tests 165 ✅ 7s ⏱️ Results for commit 3d9ee9d. ♻️ This comment has been updated with latest results. |
| ) | ||
|
|
||
| static let conversationLabel2 = ConversationLabelInfo( | ||
| id: .mockID4, |
There was a problem hiding this comment.
across tests we now use UUID helpers from WireTestingPackage
There was a problem hiding this comment.
Do we specifically need predefined ids or would it work to create random ids?
There was a problem hiding this comment.
It would work creating random ids, I figured it would be nice to harmonize the code by using our UUID helper across all tests
WireDomain/Tests/WireDomainTests/LocalStores/FeatureConfigLocalStoreTests.swift
Show resolved
Hide resolved
WireDomain/Tests/WireDomainTests/LocalStores/FeatureConfigLocalStoreTests.swift
Show resolved
Hide resolved
WireDomain/Sources/WireDomain/Repositories/Conversations/ConversationLocalStore.swift
Show resolved
Hide resolved
WireDomain/Tests/WireDomainTests/LocalStores/UpdateEventsLocalStoreTests.swift
Show resolved
Hide resolved
| public let conversationID: UUID? | ||
| public let qualifiedConversationID: WireDataModel.QualifiedID? | ||
| public let lastUpdate: Date | ||
| public let status: WireDataModel.ZMConnectionStatus |
There was a problem hiding this comment.
Are you thinking to eventually have a complete domain model by not using anything from WireDataModel?
There was a problem hiding this comment.
yes, it'd be nice to gradually move away from WireDataModel. Although a lot was factored out, we still rely a lot on it in our local stores typically using the extension helpers from the different moc. I feel like this is also related to the discussion we have regarding a WireStorage package (and the spike ticket), having a fully isolated storage layer that doesn't depend anymore on WireDataModel
| enum Error: Swift.Error { | ||
| case failedToStoreLabelLocally(UUID) | ||
| } |
There was a problem hiding this comment.
Prefer to call the nested error Failure.
| ) | ||
|
|
||
| static let conversationLabel2 = ConversationLabelInfo( | ||
| id: .mockID4, |
There was a problem hiding this comment.
Do we specifically need predefined ids or would it work to create random ids?
|
|
||
| static let lastEventIDUserDefaultsKey = "\(selfUserID.uuid.uuidString)_lastEventID" | ||
|
|
||
| nonisolated(unsafe) static let envelope1 = UpdateEventEnvelope( |
There was a problem hiding this comment.
Why is nonisolated(unsafe) needed?
There was a problem hiding this comment.
removed in ff83af9, add Sendable conformance to related value types
WireDomain/Tests/WireDomainTests/LocalStores/UpdateEventsLocalStoreTests.swift
Show resolved
Hide resolved
Key points
This PR isolates tests for our storage layer (local stores) and revisits tests for all our existing repositories.
Missing local stores were also created so the storage logic could be moved out from related repositories.
BEFORE:
AFTER:
In addition:
Testing
Checklist
[WPB-XXX].UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: